Skip to content

[18.0][MIG] account_statement_import_camt: Migration to 18.0#737

Merged
OCA-git-bot merged 22 commits intoOCA:18.0from
xaviedoanhduy:18.0-mig-account_statement_import_camt
Apr 10, 2025
Merged

[18.0][MIG] account_statement_import_camt: Migration to 18.0#737
OCA-git-bot merged 22 commits intoOCA:18.0from
xaviedoanhduy:18.0-mig-account_statement_import_camt

Conversation

@xaviedoanhduy
Copy link
Copy Markdown
Contributor

@xaviedoanhduy xaviedoanhduy commented Nov 8, 2024

@grindtildeath
Copy link
Copy Markdown

@xaviedoanhduy Can you rebase after merging other PRs?

@xaviedoanhduy xaviedoanhduy force-pushed the 18.0-mig-account_statement_import_camt branch from 180fce9 to b686079 Compare December 17, 2024 23:15
@xaviedoanhduy
Copy link
Copy Markdown
Contributor Author

@xaviedoanhduy Can you rebase after merging other PRs?

hi @grindtildeath, done!

Copy link
Copy Markdown

@sebalix sebalix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like bot commits were merged in some author commits (like unrelated translation files merged in Spanish translation contribution).

Maybe you used the feature in development there? OCA/oca-port#66 (just a guess). I would consider this feature not stable enough for the moment. IMO such commits should be squashed together (bots commits into another bot commit, to not mix contributors & contributions).

If you are unsure, better to preserve the 17.0 history as-is.

@xaviedoanhduy
Copy link
Copy Markdown
Contributor Author

Seems like bot commits were merged in some author commits (like unrelated translation files merged in Spanish translation contribution).

Maybe you used the feature in development there? OCA/oca-port#66 (just a guess). I would consider this feature not stable enough for the moment. IMO such commits should be squashed together (bots commits into another bot commit, to not mix contributors & contributions).

If you are unsure, better to preserve the 17.0 history as-is.

hi @sebalix, I don't use the feature you mentioned. I just squash the administrative commits manually following this guide: https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests#mergesquash-the-commits-generated-by-bots-or-weblate

Comment thread account_statement_import_camt/models/account_statement_import_camt_parser.py Outdated
Comment thread account_statement_import_camt/models/account_journal.py Outdated
@sebalix
Copy link
Copy Markdown

sebalix commented Mar 6, 2025

@xaviedoanhduy thanks! All good then, as soon as the commit message of contributors doesn't change it's OK (oca-port is using this criteria to detect missing commits in a target branch).

LG.

@xaviedoanhduy xaviedoanhduy force-pushed the 18.0-mig-account_statement_import_camt branch from 0a3fa7b to 8825f3a Compare March 11, 2025 05:08
@xaviedoanhduy
Copy link
Copy Markdown
Contributor Author

xaviedoanhduy commented Mar 11, 2025

please merge this PR to make the CI green: #785

@xaviedoanhduy xaviedoanhduy mentioned this pull request Mar 26, 2025
12 tasks
@BT-dmoreno
Copy link
Copy Markdown

@xaviedoanhduy the tests are failing but I don't think because of an error of the code proposed by this PR, any idea why they got stuck like that? Maybe re-running them can help in case was due to a temporal GH failure?

i-vyshnevska and others added 11 commits April 10, 2025 16:16
Currently translated at 75.0% (9 of 12 strings)

Translation: bank-statement-import-14.0/bank-statement-import-14.0-account_statement_import_camt
Translate-URL: https://translation.odoo-community.org/projects/bank-statement-import-14-0/bank-statement-import-14-0-account_statement_import_camt/nl/
Currently translated at 83.3% (10 of 12 strings)

Translation: bank-statement-import-14.0/bank-statement-import-14.0-account_statement_import_camt
Translate-URL: https://translation.odoo-community.org/projects/bank-statement-import-14-0/bank-statement-import-14-0-account_statement_import_camt/it/
Encountered in a Camt.054 statement: currency listed under
/BkToCstmrDbtCdtNtfctn/Ntfctn/Ntry/Amt/@ccy
with this commit, the narration field is filled with infos find in the camt file such as reversal indicator, return reason, cheque number, ...
before only a banking ref was present.

The transaction type field is also filled.
luc-demeyer and others added 11 commits April 10, 2025 16:16
Currently translated at 100.0% (12 of 12 strings)

Translation: bank-statement-import-16.0/bank-statement-import-16.0-account_statement_import_camt
Translate-URL: https://translation.odoo-community.org/projects/bank-statement-import-16-0/bank-statement-import-16-0-account_statement_import_camt/es/
Currently translated at 100.0% (31 of 31 strings)

Translation: bank-statement-import-16.0/bank-statement-import-16.0-account_statement_import_camt
Translate-URL: https://translation.odoo-community.org/projects/bank-statement-import-16-0/bank-statement-import-16-0-account_statement_import_camt/es/
Currently translated at 22.5% (7 of 31 strings)

Translation: bank-statement-import-16.0/bank-statement-import-16.0-account_statement_import_camt
Translate-URL: https://translation.odoo-community.org/projects/bank-statement-import-16-0/bank-statement-import-16-0-account_statement_import_camt/it/
This refactoring will allow using `TestParser._do_parse_test()` method for subclasses defined in inheriting modules.

This way, if the new module defines new files in other paths than `account_statement_import_camt/test_files/`,
 subclasses of `TestParser` will still be able to use the `_do_parse_test()` method for imports and checks.

Moreover, a custom value can now be defined for the acceptable differing lines number between inputfile and goldenfile.

It will also maintain backward compatibility with the current usage of the method
 (ie: using only a filename with the assumption of finding it in `account_statement_import_camt/test_files/`).

NB: methods are now defined outside of `TestParser` to allow importing them directly, instead of inheriting from `TestParser`.
@xaviedoanhduy xaviedoanhduy force-pushed the 18.0-mig-account_statement_import_camt branch from 8825f3a to 1334327 Compare April 10, 2025 09:16
@BT-dmoreno
Copy link
Copy Markdown

@OCA/banking-maintainers could you please take a look at this and merge it if you consider, thanks!

Copy link
Copy Markdown
Member

@etobella etobella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically looks good. The relevant changes are related to transformation from % to format.

@OCA-git-bot
Copy link
Copy Markdown
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@etobella
Copy link
Copy Markdown
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Copy Markdown
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 18.0-ocabot-merge-pr-737-by-etobella-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 9e1ac84 into OCA:18.0 Apr 10, 2025
7 checks passed
@OCA-git-bot
Copy link
Copy Markdown
Contributor

Congratulations, your PR was merged at 12a9b2e. Thanks a lot for contributing to OCA. ❤️

@BT-dmoreno
Copy link
Copy Markdown

Thanks @xaviedoanhduy for having pushed those commits a while ago and being pro-active 🥇.

# pylint: disable=except-pass
except (zipfile.BadZipFile, ValueError):
pass
_logger.exception("BadZipfile exception")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is an exception logged now instead of ignored? This code will run on any file that is not a valid camt file, i.e. any file of any other format supported by the import framework.

BhaveshHeliconia pushed a commit to HeliconiaIO/bank-statement-import that referenced this pull request Mar 20, 2026
Signed-off-by etobella
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.